-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
@nguerrera FYI |
@@ -0,0 +1,31 @@ | |||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The headers aren't right for corefx. I think these files should be relicensed to MIT. cc @richlander
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'll write a script to update them each time I sync between Roslyn and CoreFx.
This is very cool! It's also huge so I will need considerably more time to review in detail. I only scanned so far. I am conceptually OK merging early to unblock upstream work and feedback channel, but corefx leads should chime in: cc @joshfree, @weshaggard. I know we had broad agreement to approach this feature that way, but I want to make sure they're aware of the scope of this change. Tests should be given high priority and I think it would be worth doing the API review ASAP (cc @terrajobst). Please file an issue to track the API review and follow-up. |
Re Tests - yes, I'm definitely gonna move all relevant unit tests from Roslyn (and add more). I haven't done that yet since I need to keep the code in sync between Roslyn and CoreFx for now and doing so for tests as well is just more overhead. Once we pass the requirements for taking dependency on this new implementation in Roslyn master, we can finish the move and delete the code from Roslyn. BTW, I run every change in this code thru Roslyn CI and everything has been passing so far. So this code is well covered (though indirectly). |
67f6985
to
465e49d
Compare
OpenSUSE and Ubuntu failures are unrelated. Merging. |
PEBuilder implementation Commit migrated from dotnet/corefx@a433613
An initial implementation taken from Roslyn (PR: dotnet/roslyn#7683)
The API is still very much in flux with known issues. Having it in System.Metadata.Reader beta build however allows partners to start building on top of it and provide feedback.
Usage sample (emitting "Hello world" .exe):
https://github.com/dotnet/roslyn/blob/1195fb186b67474edbc39baf40fdbcbaf667b892/src/Compilers/Core/CodeAnalysisTest/PEWriter/PEBuilderTests.cs